-
-
Notifications
You must be signed in to change notification settings - Fork 34.6k
src: initial support for ESM in embedder API #61548
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Review requested:
|
|
cc @nodejs/embedders |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61548 +/- ##
==========================================
- Coverage 89.77% 89.77% -0.01%
==========================================
Files 673 673
Lines 203820 204045 +225
Branches 39175 39194 +19
==========================================
+ Hits 182987 183183 +196
- Misses 13152 13193 +41
+ Partials 7681 7669 -12
🚀 New features to boost your workflow:
|
|
I don't think this is semver-major - notice none of the existing tests need updating, as the new overloads are backward compatible. (I would be worth at some point deprecating the older overloads, maybe before March for 26, because the older types are prone to ABI breakage whenever we try to add more features, but that can be a separate PR). |
|
hmm, noticed that I accidentally changed the original overloads - updated to remove the changes. This should be now fully backward compatible. |
addaleax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one question, otherwise this looks awesome! Thank you @joyeecheung! ✨
This patch extends `LoadEnvironment` to support loading ES modules,
and adds the following new types:
```cpp
enum class ModuleFormat : uint8_t {
kCommonJS,
kModule,
};
// Data for specifying an entry point script for LoadEnvironment().
// This class uses an opaque layout to allow future additions without
// breaking ABI. Use the setter methods to configure the entry point.
class ModuleData {
void set_source(std::string_view source);
void set_format(ModuleFormat format);
void set_resource_name(std::string_view name);
std::string_view source() const;
ModuleFormat format() const;
std::string_view resource_name() const;
};
class StartExecutionCallbackInfoWithModule {
void set_env(Environment* env);
void set_process_object(v8::Local<v8::Object> process_object);
void set_native_require(v8::Local<v8::Function> native_require);
void set_run_module(v8::Local<v8::Function> run_module);
void set_data(void* data);
Environment* env();
v8::Local<v8::Object> process();
v8::Local<v8::Function> native_require();
v8::Local<v8::Function> run_module();
void* data();
};
```
And two new `LoadEnvironment()` overloads:
```cpp
// Run entry point with ModuleData configuration
MaybeLocal<Value> LoadEnvironment(
Environment* env,
const ModuleData* entry_point,
EmbedderPreloadCallback preload = nullptr);
// Callback-based with new StartExecutionCallbackInfoWithModule
MaybeLocal<Value> LoadEnvironment(
Environment* env,
StartExecutionCallbackWithModule cb,
EmbedderPreloadCallback preload = nullptr,
void* callback_data = nullptr);
```
| auto info = CallbackInfoFromArray(env, result); | ||
| if (!info.has_value()) { | ||
| MaybeLocal<Value>(); | ||
| return MaybeLocal<Value>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drive-by: I noticed that this was likely a bug and fixed it while I was at it (this only gets hit if somehow extracting the arguments from the JS array returned by bootstrap scripts throws, which is extremely unlikely though).
|
Rebased and addressed the comments in the last commit. Can you take a look again? Thanks! @addaleax @Aditi-1400 |
This patch extends
LoadEnvironmentto support loading ES modules, and adds the following new types:And two new
LoadEnvironment()overloads:Notes about the
run_modulefunction:run_cjsfunction is renamed torun_moduleand now acceptsthree arguments:
(source, format, resourceName)where formatcorresponds to
ModuleFormatvalues. This keeps the oldStartExecutionCallbackbackward-compatible, and reuses this newfunction for
StartExecutionCallbackWithModule.points continue to return the wrapper's return value.
The following are left as TODO for follow-up PRs.
import()andimport.metaRefs: #53565